Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: CI pipeline to build, package, test, and deploy each merged PR #912

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

jcrossley3
Copy link
Contributor

@jcrossley3 jcrossley3 commented Oct 9, 2024

Fixes: #804

Requires an OPENSHIFT_TOKEN secret that enables privileged access to an OCP instance, e.g. https://api.cluster.trustification.rocks:6443

The following commands were used to generate the token on the OCP host:

oc create serviceaccount ${ACCT_NAME}
oc create clusterrolebinding ${ACCT_NAME} --clusterrole=cluster-admin --serviceaccount=default:${ACCT_NAME}
oc create token ${ACCT_NAME} --duration=$((365*24))h

Upon successful completion of the latest-release workflow, the app will be available here:
https://trustify-latest-staging.apps.cluster.trustification.rocks/

Fixes: trustification#804

Requires an OPENSHIFT_TOKEN secret that enables privileged access to
an OCP instance, e.g. https://api.cluster.trustification.rocks:6443

The following commands were used to generate the token on the OCP
host:

```
oc create serviceaccount ${ACCT_NAME}
oc create clusterrolebinding ${ACCT_NAME} --clusterrole=cluster-admin --serviceaccount=default:${ACCT_NAME}
oc create token ${ACCT_NAME} --duration=$((365*24))h
```

Signed-off-by: Jim Crossley <[email protected]>
@jcrossley3
Copy link
Contributor Author

You can view the results of the workflow on my personal branch: https://github.com/jcrossley3/trustify/actions/runs/11264784849

runs-on: ubuntu-22.04
needs:
- publish
- e2e-test
Copy link
Member

@carlosthe19916 carlosthe19916 Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcrossley3

  1. I wanted to point our attention to the fact that we will deploy the code only if the e2e tests passed. I think we should deploy it regardless of the e2e pass or not. Actually, I think the e2e test might or might not fail. Just yesterday you faced an e2e error due to the fact that the the UI of the backend had an outdated version. But I do not hold a strong opinion so I'll leave that to you.
  2. I thought the idea was to deploy the downstream version of Trustify not the upstream one. That might be just me not getting the requirement correctly though. In any case this is a good milestone!
  3. I created this issue Do not delete the NS and let the user do it by himself trustify-operator#34 in regards to the fact that you made the trustify-operator scripts to delete the namespace. I think that is a dangerous thing to be set in the trustify-operator scripts and the action of deletion should be done by the user itself (a conscious decision). So Just like you are using redhat-actions/oc-login@v1 to login in OCP you should delete the Namepace in a command defined in this workflow.
  • oc login (uses: redhat-actions/oc-login@v1)
  • oc delete project MY_PROJECT
  • Install the oprator uses: trustification/trustify-operator/.github/actions/install-trustify@main

Copy link
Contributor Author

@jcrossley3 jcrossley3 Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I wanted to point our attention to the fact that we will deploy the code only if the e2e tests passed. I think we should deploy it regardless of the e2e pass or not. Actually, I think the e2e test might or might not fail. Just yesterday you faced an e2e error due to the fact that the the UI of the backend had an outdated version. But I do not hold a strong opinion so I'll leave that to you.

Interesting. I think of a CI pipeline as sequential (build, package, test, deploy, etc), failing immediately if any step fails. I like knowing that the thing deployed has passed all our tests, at least the ones known at the time. In the case of the failed ui tests yesterday, we'd only know about that after merging a PR in this repo, which isn't great, but at least we'd know about it eventually, in an automated way.

But if there's a majority opinion either way, I'm happy to abide.

  1. I thought the idea was to deploy the downstream version of Trustify not the upstream one. That might be just me not getting the requirement correctly though. In any case this is a good milestone!

Easy enough to do -- now that I know how -- but right now both upstream and downstream are identical, except for the konflux config, which isn't buying us anything yet.

Personally, I like for the deployment of the latest thing merged to come out of the repo that merged it.

  1. I created this issue Do not delete the NS and let the user do it by himself trustify-operator#34 in regards to the fact that you made the trustify-operator scripts to delete the namespace. I think that is a dangerous thing to be set in the trustify-operator scripts and the action of deletion should be done by the user itself (a conscious decision). So Just like you are using redhat-actions/oc-login@v1 to login in OCP you should delete the Namepace in a command defined in this workflow.

Yeah, I commented on that issue. Deleting the namespace was a hack that gave me a crude, idempotent behavior. And I didn't think it would affect anything you were currently using that script for, because you're always using it in a "virgin" minikube. With a long-lived OCP instance, we don't have the luxury of assuming we can install everything every time.

  • oc login (uses: redhat-actions/oc-login@v1)
  • oc delete project MY_PROJECT
  • Install the oprator uses: trustification/trustify-operator/.github/actions/install-trustify@main

To be clear, I don't want to delete the namespace. I want the operator to install a new version of trustify into that namespace. This would significantly reduce the time the app is down. This is the way operators should work, imo, but I need help refactoring that script to achieve that behavior.

Copy link
Member

@carlosthe19916 carlosthe19916 Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I don't want to delete the namespace. I want the operator to install a new version of trustify into that namespace. This would significantly reduce the time the app is down. This is the way operators should work, imo, but I need help refactoring that script to achieve that behavior.

Agree. Deleting the NS also means deleting the whole data that was before. I don't want to block your PR, it has been approved, feel free to merge it. But it will be worth investigating how to achieve what you mentioned. Not sure I have too much time to support you in that journey though, but I'll try to find some free time to also suggest some solutions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, I'll mention what I mentioned somewhere in other thread:

This is how Operators work.

  • Step 1: The operator is released
  • Step 2: you install your operator in OCP
  • Step 3: the team releases a new version of the backend (which included the UI) every night.
  • Step 4: each night the a new version of the operator is realeased
  • Step 5: OLM detects there is a new operator released and automatically updates the container images used in previous release and updates the application to the latest one.

Important to note that the Operator is installed only once in STEP 2.

This is how a real application receives updates when they install an operator. E.g. Today my company installs the Keycloak Operator in my OCP instance and in 1 month the Keycloak teams releases a new version of Keycloak (also the operator). ME as a company should not move a finger to receive updates from Keycloak as my operator, through OLM, should update the Keycloak version as soon as there is a new one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That magic happens when you install an operator and enable the AUTOMATIC upgrades monitor:

ocp-operator-install-details1

That way, OLM does the magic and you don't need to actually deploy the whole thing after each commit.

But for that to happen a requirement would be:

  • The deployment is done nightly and not after each PR
  • Nightly a new version is released under a K8s channel development and it takes the latest version of container

That is just an idea, feel free to discard it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all sounds good to me, and resonates with what I recall from working with operators in the early days (pre bundles) back when they rarely worked. 😉

  • The deployment is done nightly and not after each PR

Why? What does the frequency matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? What does the frequency matter?

I think yes. But don't take my word for granted, I can be wrong.

For the automatic updates to work 2 things need to happen. A new container version of this repository needs to be created. And second, a new version of the trustify-operator needs to be released at https://github.com/redhat-openshift-ecosystem/community-operators-prod/pulls .

Since we have those 2 conditions, releasing for each commit would mean:

They won't have the OPENSHIFT_TOKEN

Signed-off-by: Jim Crossley <[email protected]>
@jcrossley3 jcrossley3 added this pull request to the merge queue Oct 11, 2024
Merged via the queue into trustification:main with commit feac95e Oct 11, 2024
1 check passed
@jcrossley3 jcrossley3 deleted the 804 branch October 11, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2 CI/CD pipeline and deployment to Trustify Staging
3 participants